Validate cross-referenced parameters between PostgresCluster and PostgresClusterClass by limak9182 · Pull Request #1858 · splunk/splunk-operator

limak9182 · 2026-04-20T13:48:37Z

Description

Adds admission-time cross-resource validation for PostgresCluster against its referenced PostgresClusterClass
Uses mgr.GetAPIReader() (live API read, not cached) to avoid false rejections during cache propagation

Key Changes

Rule	Field	Condition
Class existence	`spec.class`	Referenced PostgresClusterClass must exist
Storage floor	`spec.storage`	Cannot be lower than class default
Version floor	`spec.postgresVersion`	Major version cannot be lower than class default. If minor version specified in class cluster's minor cannot be lower.
Pooler enablement	`spec.connectionPoolerEnabled`	Cannot be `true` when class has it disabled
Merge completeness	`spec.instances`, `spec.postgresVersion`, `spec.storage`	After overlay, all three must be resolvable from cluster or class

Testing and Verification

unit test cases with fake client.Reader covering all rules and edge cases
Existing pg_hba validation tests updated to pass nil reader (no behavioral change)
go build ./... passes
go test ./pkg/splunk/enterprise/validation/... passes

Related Issues

Jira tickets, GitHub issues, Support tickets...

PR Checklist

Code changes adhere to the project's coding standards.
Relevant unit and integration tests are included.
Documentation has been updated accordingly.
All tests pass locally.
The PR description follows the project's guidelines.

github-actions · 2026-04-20T13:48:51Z

CLA Assistant Lite bot:
Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you sign our Contribution License Agreement before we can accept your contribution. You can sign the CLA by just posting a Pull Request Comment with the exact sentence copied from below.

I have read the CLA Document and I hereby sign the CLA

_{You can retrigger this bot by commenting recheck in this Pull Request}

DmytroPI-dev · 2026-04-22T12:00:45Z

+			wantMessage: "PostgresClusterClass not found",
+		},
+		{
+			name: "rejected - storage below class floor",


storage below class floor - I assume we can override this value on a cluster lever even with lower ones, am I wrong?

yes, correct

DmytroPI-dev · 2026-04-22T12:01:56Z

+			wantMessage: "storage cannot be lower than class default",
+		},
+		{
+			name: "rejected - version below class floor",


version below class floor - same as above - can't we set lower versions on cluster level now?

For the compliance we prohibit setting lower version than in class

DmytroPI-dev · 2026-04-22T12:03:00Z

+				},
+			},
+			wantAllowed: false,
+			wantMessage: "connectionPoolerEnabled cannot be enabled",


"connectionPoolerEnabled cannot be enabled" - sounds strange (enabled cannot be enabled 😸 )

DmytroPI-dev · 2026-04-22T12:04:05Z

+			wantMessage: "connectionPoolerEnabled cannot be enabled",
+		},
+		{
+			name: "allowed - storage equal to class",


"allowed - storage equal to class" - and if higher?

DmytroPI-dev · 2026-04-22T12:04:48Z

 // ValidatePostgresClusterUpdate validates a PostgresCluster on UPDATE.
-func ValidatePostgresClusterUpdate(obj, oldObj *enterpriseApi.PostgresCluster) field.ErrorList {
-	return ValidatePostgresClusterCreate(obj)
+func ValidatePostgresClusterUpdate(obj, oldObj *enterpriseApi.PostgresCluster, reader client.Reader) field.ErrorList {


oldObj is ignored on purpose?

yes, we don't care about oldObj

DmytroPI-dev · 2026-04-22T12:28:22Z

+		return allErrs
+	}
+
+	if obj.Spec.Storage != nil && classConfig.Storage != nil {


Same point for storage and postgresVersion: the merge code treats class values as defaults, but this webhook now treats them as minimum allowed values. If that policy shift is intentional, I’d update the API docs/comments because “overrides” reads differently today.

I think there is no contradiction here i.e our default logic is there and it is used in the runtime reconciler but business rules validation is done here in the validation webhook. So we have "overrides" with guardrails in place. I agree we need to document this logic properly though

added comment for clarity

DmytroPI-dev · 2026-04-22T12:29:58Z

+	class := &enterpriseApi.PostgresClusterClass{}
+	if err := reader.Get(context.Background(), client.ObjectKey{Name: obj.Spec.Class}, class); err != nil {
+		allErrs = append(allErrs, field.Invalid(
+			field.NewPath("spec").Child("class"),
+			obj.Spec.Class,
+			"referenced PostgresClusterClass not found"))
+		return allErrs
+	}


reader.Get() collapses every error into “referenced PostgresClusterClass not found”. That will hide real issues like API/RBAC failures and return a misleading validation error. I’d special-case IsNotFound and surface other errors separately.

removed API reader

DmytroPI-dev · 2026-04-22T12:31:41Z

+	if k8sClient != nil || reader != nil {
+		vc = NewValidationContext(k8sClient, reader, req.Namespace)
 	}


Cross-resource validation now depends entirely on vc.Reader. If a caller provides only a cached client, or calls the validator directly, the checks silently disappear. I’d prefer a Reader -> Client fallback so the behavior is less wiring-sensitive.

removed API reader

DmytroPI-dev · 2026-04-22T12:32:44Z

+		},
+	}
+
+	for _, tt := range tests {


The new tests cover the happy path well, but they mostly assert error count/field or Allowed. I’d add message assertions for the unit tests and one update-path integration case for the cross-resource checks, so semantic regressions in the comparison logic are easier to catch.

DmytroPI-dev · 2026-04-22T12:33:14Z

+	return b
+}
+
+func TestCrossResourceValidationIntegration(t *testing.T) {


The new tests cover the happy path well, but they mostly assert error count/field or Allowed. I’d add message assertions for the unit tests and one update-path integration case for the cross-resource checks, so semantic regressions in the comparison logic are easier to catch.

mploski · 2026-04-23T11:21:43Z

 			ReadTimeout:  readTimeout,
 			WriteTimeout: writeTimeout,
 			Client:       mgr.GetClient(),
+			Reader:       mgr.GetAPIReader(),


could you elab more, why we need this reader? What problem do we solve?

mploski · 2026-04-23T11:49:04Z

 		}
 	}

+	if reader != nil {


could you elab why we expect some reader to validate against class logic?

mploski · 2026-04-23T12:07:26Z

+	// Reader is a live API reader that bypasses the cache.
+	// Use this for cross-resource lookups where eventual consistency
+	// could cause false rejections.
+	Reader client.Reader


Is this really a problem in our case? if we for example deploy new cluster CR and we need to check latest deployed clusterClass isnt it already populated within the cache? Why we need to bypass it? Why this isnt a problem for other CR's checked by this?

limak9182 force-pushed the feature/pg_hba-validation branch from dbafebc to fb47985 Compare April 22, 2026 07:22

limak9182 force-pushed the feature/cross-referenced-parameters-validation branch from 21145a6 to 74dca92 Compare April 22, 2026 10:53

DmytroPI-dev reviewed Apr 22, 2026

View reviewed changes

mploski reviewed Apr 23, 2026

View reviewed changes

Base automatically changed from feature/pg_hba-validation to feature/database-controllers April 23, 2026 13:50

limak9182 force-pushed the feature/cross-referenced-parameters-validation branch from 62dfaba to 19b975f Compare April 27, 2026 07:19

limak9182 added 10 commits April 27, 2026 09:39

pg_hba validation

f27a666

comment fix

0580d68

integration tests

fd0611d

fix tests

dd96323

structure fix

213ab5b

fix tests

d7e6ab3

docs fix

a850bda

cross cluster and class validation

1d863e9

minnor version check

0c51361

remove API reader

96b28d3

limak9182 force-pushed the feature/cross-referenced-parameters-validation branch from 19b975f to 96b28d3 Compare April 27, 2026 07:40

tests fix

7622554

limak9182 closed this Apr 27, 2026

github-actions Bot locked and limited conversation to collaborators Apr 27, 2026

Conversation

limak9182 commented Apr 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Key Changes

Testing and Verification

Related Issues

PR Checklist

Uh oh!

github-actions Bot commented Apr 20, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mploski Apr 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

limak9182 commented Apr 20, 2026 •

edited

Loading

mploski Apr 23, 2026 •

edited

Loading